Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added render class for Viewfield. #4

Closed
wants to merge 7 commits into from

Conversation

marcus-n3rd
Copy link

Absorbed the following functions into the class:

  • viewfield_element_info()
  • viewfield_pre_render()
  • viewfield_post_render()
  • _viewfield_get_view_args()
    -- This was made getViewArgs() which is a protected method

Left rendering for the theme, i.e.: removed $output = render ($view_el);, other than that I didn't touch any logic within the functions.

Thank Metal Toad for paying my hours for this. Lightly tested this on a client's project and I'd say it's now eligible to go from sandbox to full project status.

Lukas von Blarer and others added 5 commits June 9, 2016 03:55
Absorbed the following functions into the class:
- viewfield_element_info()
- viewfield_pre_render()
- viewfield_post_render()
- _viewfield_get_view_args()
-- This was made getViewArgs() which is a protected method

Left rendering for the theme.
* adding-cache-tags:
  Using buildRenderable() to add cache metadata
  Remove kint()
  Adding cache tags

# Conflicts:
#	viewfield.module
This is so that the arguments have had an opportunity to get aggregated by the Viewfield class.
@marcus-n3rd
Copy link
Author

I combined the changes from:

Copy link
Collaborator

@derhasi derhasi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@marcus-n3rd, thanks for your work. I'm just starting getting into the issues again after some months.

Here are some issues I found from a "meta" perspective. I will have a closer look at the functionality itself later.

* Contains \Drupal\Core\Render\Element\Viewfield.
*/

namespace Drupal\Core\Render\Element;
Copy link
Collaborator

@derhasi derhasi Sep 30, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It should not use core's namespace, but Drupal\viewfield\... instead.

*/
class Viewfield extends RenderElement {

public function getInfo() {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This needs at least function documentation. I guess {@inheritdoc} should do its job here.

);
}

public static function preRenderViewfield($element) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This needs some proper function documentation and type declaration.

return $element;
}

public static function postRenderViewfield($content, $element) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This needs some proper function documentation and type declaration too.

/**
* Perform argument replacement
*/
protected function getViewArgs($vargs, $entity_type, $entity) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There needs to be some parameter documentation.

@marcus-n3rd
Copy link
Author

Oh fudge. While step debugging through in order to double check the argument types, I found that none of Viewfield class is actually used. 🤦 I'm going to run through and see if there are parts that can be consumed and used in the other classes.

@marcus-n3rd
Copy link
Author

I am really pondering the value of having it as a RenderElement. Could the functionality just be moved into the ViewfieldDefaultFormatter (with the theme name changing to simply viewfield rather than viewfield_formatter_default)? Is the viewfield_stack static variable still needed with Drupal 8's render caching, because that would get rid of the need of the pre/post rendering functions/methods?

@luksak
Copy link

luksak commented Oct 14, 2016

Well, I was trying to fix the caching issues and added that in the field formatter. As Berdir suggested, we should move that somewhere else where the render array is being built and that is what you approach does, right?

@marcus-n3rd marcus-n3rd mentioned this pull request Oct 24, 2016
@marcus-n3rd
Copy link
Author

Closing in preference of #8.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants